-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue#743: fetch.ubuntu._run_with_retries raises on unexpected exitcode #744
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@kwmonroe do you like green tests? Separate PR or can they be merged together |
request_time = '2021-02-02 10:19:55' | ||
timestamp = datetime.datetime.strptime( | ||
'Tue ' + request_time + ' UTC', | ||
'%a %Y-%m-%d %H:%M:%S %Z').timestamp() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the timestamp is seconds since epoch -- but it's timezone specific. This change was so this test would run successfully on all machines regardless of the locale's timezone. (ie -- it failed on my machine but was okay in other places where there was no TZ set)
@mock.patch.object(hookenv, '_run_atstart') | ||
@mock.patch.object(hookenv, 'config', mock.Mock()) | ||
def test_manage_stop(self, _run_atstart, hook_name, stop_services, reconfigure_services): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjust most of the test_manage_*
tests such that they don't ACTUALLY run _run_atstart
-- but use a mock instead
assert not stop_services.called | ||
reconfigure_services.assert_called_once_with() | ||
provide_data.assert_called_once_with() | ||
|
||
@mock.patch.object(hookenv, '_atstart', []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except for the test_manage_calls_atstart
which guarantees hookenv._atstart
is empty before the test runs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nice, and I appreciate the focus on green tests for py310. I'm confused how this 3.10 failure didn't show up in py <= 3.8:
ERROR: test_manage_calls_atexit (tests.core.test_services.TestServiceManager)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/core/hookenv.py", line 1180, in inner_translate_exc2
return f(*args, **kwargs)
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/core/hookenv.py", line 1218, in is_leader
return json.loads(subprocess.check_output(cmd).decode('UTF-8'))
File "/opt/hostedtoolcache/Python/3.10.8/x64/lib/python3.10/subprocess.py", line [42](https://github.com/juju/charm-helpers/actions/runs/3331110013/jobs/5510456204#step:7:43)1, in check_output
return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
File "/opt/hostedtoolcache/Python/3.10.8/x64/lib/python3.10/subprocess.py", line 503, in run
with Popen(*popenargs, **kwargs) as process:
File "/opt/hostedtoolcache/Python/3.10.8/x64/lib/python3.10/subprocess.py", line 971, in __init__
self._execute_child(args, executable, preexec_fn, close_fds,
File "/opt/hostedtoolcache/Python/3.10.8/x64/lib/python3.10/subprocess.py", line 1847, in _execute_child
raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'is-leader'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/runner/work/charm-helpers/charm-helpers/tests/core/test_services.py", line 81, in test_manage_calls_atexit
manager.manage()
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/core/services/base.py", line 130, in manage
hookenv._run_atstart()
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/core/hookenv.py", line 13[48](https://github.com/juju/charm-helpers/actions/runs/3331110013/jobs/5510456204#step:7:49), in _run_atstart
callback(*args, **kwargs)
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/coordinator.py", line 3[59](https://github.com/juju/charm-helpers/actions/runs/3331110013/jobs/5510456204#step:7:60), in handle
if not hookenv.is_leader():
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/core/hookenv.py", line 1182, in inner_translate_exc2
raise to_exc
NotImplementedError
Perhaps a nose
vs nose2
change. Perhaps gremlins in the 3.8 vs 3.10 basepython. At any rate, i'm comfortable with the current impl and think passing 3.10 outweighs why <= 3.8 weren't always failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
this method gives a false success when in reality it should fail immediately (no retrying)
the method should raise when the exit value of
subprocess.check_call
is an error not on the rety listTLDR:
if its exit value is 0 -- 🎉
if the exit value is in the
retry_results
-- retry max_retries like expectedif the exit value is not in the
retry_results
-- 🔥Addresses #743